-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable collection integration tests on GHA #14397
Conversation
d686ebc
to
181ca6b
Compare
|
||
- uses: ./.github/actions/upload_awx_devel_logs | ||
|
||
collection-integration-coverage-combine: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that the previous job produces coverage data... what is this combining with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So:
- The matrix jobs will each produce a bunch of coverage files.
- They upload them as an artifact (I wish there were a better way to do that, I don't really want those to stay around as artifacts, they aren't very useful to most people... but we need them all to generate a report.)
- Then this job grabs all the coverage files from all the matrix jobs, shoves them into a place where
ansible-test
knows how to work with them, and then asksansible-test
to combine them into a single file and generate a report from that file.
It kind of has to be done this way because pycoverage
has to know where to look for the actual source code to include it in the reports. And ansible-test
does some hackery to make that all work out nicely for collections.
run: | | ||
echo "${{ inputs.github-token }}" | docker login ghcr.io -u ${{ github.actor }} --password-stdin | ||
|
||
- name: Pre-pull image to warm build cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you're just running out of that image, not just warming up your cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That comment/name came from the original e2e_tests, IIRC. But yeah, valid, I can change it.
Actually, I think it'd be really cool to cache the database every once in a while. Then just let things fetch a relatively recent database and apply the latest few migrations on top of it.
But that's a rainy-day idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I've updated it so that it builds the image each time now from the current checkout.
I guess we do want that, even here for the collection tests (consider: developing a feature that also has collection changes, we want to test them in lockstep). But since it adds so much time, I might split up the job matrix a bit more to try to get the time back down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking more on this and doing some more research -- we're using buildkit's inline caching which only caches the last stage and we use a multistage build. So I think we don't really get to take advantage of the caching here.
I'd like to propose (or at least explore and discuss) making our "official" ghcr.io devel images (the ones that get built on pushes to devel and feature branches) use a registry cache instead. Basically they would push a secondary cache image alongside the branch image.
So we'd have (for example): ghcr.io/ansible/awx_devel:devel
and ghcr.io/ansible/awx_devel:devel_cache
or something.
I can't guarantee this will speed things up, but I think there's a chance that it could.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance, I don't think I disagree with your proposal for use of the registry cache, but we certainly want to keep that out of the current scope and I'd like @shanemcd to have a look. What we already have has been working relatively well to keep build times fast and there are sometimes slowdown you see due to out-of-date branches or dependency changes that don't apply to small patches and aren't a real problem.
@AlanCoding thanks for the review/comments. I am planning to file a few issues for future improvements, the main one that comes to mind is nixing our These do basically the exact same thing as the new action. Let's standardize on one way to set up the dev env in CI. |
When I introduced the |
181ca6b
to
5727825
Compare
I don't think it would at all, we can just pass in another variable with
I wasn't sure what you meant by this. |
The
Something's not clicking for me here, so I'm going to talk in fairly basic terms. Some PRs will modify our dependencies and make corresponding API code changes (and errors will happen if image & code is out-of-sync) - this is basically guaranteed to happen. Some PRs will modify the collection. It doesn't make sense to run unconditional tests without first building the image, because that would almost guarantee false-failures whenever something big happens. So running the tests unconditionally (which I'm not telling you to do) forces the issue of building the image. I would be in favor of both making this conditional to |
3a18560
to
9598937
Compare
There are a number of changes here: - Abstract out a GHA composite action for running the dev environment - Update the e2e tests to use that new abstracted action - Introduce a new (matrixed) job for running collection integration tests. This splits the jobs up based on filename. - Collect coverage info and generate an html report that people can download easily to see collection coverage info. - Do some hacks to delete the intermediary coverage file artifacts which aren't needed after the job finishes. Signed-off-by: Rick Elrod <[email protected]>
cbf3530
to
937343f
Compare
SUMMARY
There are a number of changes here:
NOTE We should make the new checks be non-voting for a week or two, and make sure they don't have intermittent flakes.
ISSUE TYPE
COMPONENT NAME